-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: 케이크샵 인증 요청 리스트 API 개발 #183
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #183 +/- ##
=============================================
+ Coverage 87.92% 88.78% +0.86%
- Complexity 315 325 +10
=============================================
Files 105 109 +4
Lines 952 972 +20
Branches 37 37
=============================================
+ Hits 837 863 +26
+ Misses 97 92 -5
+ Partials 18 17 -1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
a36668d
to
21686d3
Compare
21686d3
to
2cc2d6e
Compare
2cc2d6e
to
f329b29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 방식이라고 생각은 듭니다만, VerificationPolicy 객체를 엔티티로 구성하여 저장해놓는게 어떨까요? 인증 요청을 business_information에 저장하는게 조금 부자연스럽네요
@Column(name = "business_registration_image_url", length = 200) | ||
private String businessRegistrationImageUrl; | ||
|
||
@Column(name = "id_card_image_url", length = 200) | ||
private String idCardImageUrl; | ||
|
||
@Column(name = "emergency_contact", length = 20) | ||
private String emergencyContact; | ||
|
||
@ColumnDefault("0") | ||
@Convert(converter = VerificationStatusConverter.class) | ||
@Column(name = "verification_status", nullable = false) | ||
private VerificationStatus verificationStatus = VerificationStatus.PENDING; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인증 요청 관련해서 따로 테이블 빼는게 좋지 않을까요?
cakk-domain/mysql/src/main/java/com/cakk/domain/mysql/bo/user/VerificationStatus.java
Outdated
Show resolved
Hide resolved
cakk-domain/mysql/src/main/java/com/cakk/domain/mysql/bo/user/VerificationStatus.java
Outdated
Show resolved
Hide resolved
@Component | ||
public class DefaultVerificationPolicy implements VerificationPolicy { | ||
|
||
@Override | ||
public boolean isCandidate(User user, VerificationStatus verificationStatus) { | ||
return !user.isBusinessOwner() && verificationStatus.isCandidate(); | ||
} | ||
|
||
@Override | ||
public VerificationStatus approveToBusinessOwner(User user) { | ||
user.upgradedBusinessOwner(); | ||
return VerificationStatus.makeApproved(); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단방향을 구성하기 위한 접근은 좋은데, 해당 객체가 아닌 요청 관련해서 따로 도메인을 분리하여 관리하는건 어떤가요?
cakk-api/src/main/java/com/cakk/api/dto/param/operation/OwnerCandidateParam.java
Show resolved
Hide resolved
이 부분에 대해서 저도 고민을 많이 해봤는데, 이것도 하나의 트레이드오프이지 않을까 싶어요 CakeShop과 BusinessInformation에서 정규화 작업은 크게 user, businessNumber 데이터를 분리했다고 생각합니다 그렇기 때문에 "사업자 인증 요청 처리" 라는 작업과 조회에 대한 책임을 부여한 설계로 지금 리팩토링 의도를 담았습니다 물론 의견을 주신대로 수행하면 데이터베이스 측면에서는 정규화의 이점은 있다고 생각합니다 하지만 VerificationPolicy 객체를 엔티티로 구성하면 백오피스 API이긴 하나, ORM을 쓰면서 지연 로딩에 신경 쓸 부분이 많아지고 트랜잭션의 경계가 총 User, CakeShop, BusinessInformation, VerificationPolicy 4개의 태이블에 대한 Lock을 생각해야 된다고 여겨져서 현재 상황을 유지해보고 명확한 이유가 생겼을 때 분리해보고 싶습니다! 또한 VerificationPolicy를 추상화하여서 현재 DefaultVerificationPolicy 구현체를 활용하고 있는데 |
추가로, 코멘트에 남겨둔 해결해야 할 트레이드 오프 에 대해서도 의견 주시면 감사합니다 - 의견에 따라 로직도 다음PR에서 살짝 변경해야 할 것 같아요 |
@Column(name = "business_registration_image_url", length = 200) | ||
private String businessRegistrationImageUrl; | ||
|
||
@Column(name = "id_card_image_url", length = 200) | ||
private String idCardImageUrl; | ||
|
||
@Column(name = "emergency_contact", length = 20) | ||
private String emergencyContact; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
새롭게 추가한 컬럼에 대한 초기화는 없나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
케이크샵 생성할 때 businessInformation도 같이 생성되어야 해서
user포함 새롭게 추가한 컬럼 nullable에 대해 true로 허용되어야할 것 같습니다
PR 트레이드 오프
일단 확인했습니다. 다만, slack api에 문제가 있어서 요청 데이터를 손실한다면 혹시 백업되고 있나요? |
이 부분 백오피스 api 개발 끝나고 PR새롭게 올리겠습니다 |
백오피스 API 중, 케이크샵 인증 요청 리스트 목록 조회(아직 처리되지 않은 인증) API 개발
Domain Model Pattern에서 Module간의 응집도가 너무 떨어져서 비즈니스 로직을 재설계하였습니다
- 첫 번째 이슈: 아직 처리되지 않은 상태에 대해서 어떤 값을 가질까?
현재 응집도가 너무 떨어지는 부분은 CakeShop이라는 도메인이 사업자인증이 완료됐는지, 아닌지에 대한 책임을 가지는 부분이 논리적인 오류가 있습니다. 이를 해결하기 위해 BusinessInformation이 책임을 가지도록 재설계 했습니다.
- 두 번째 이슈: 인증이 완료 또는 인증되지 않음 2가지 상태만 가질 수 있을까?
이를 위해 PENDING(보류), APPROVED(인증 완료), REJECTED(거절) 에 대한 3가지 상태로 확장하여 비즈니스 로직을 재설계했습니다.
- 세 번째 이슈: 인증 요청 리스트 목록을 통해 조회 될 데이터가 저장되고 있지 않음
이를 해결하기 위해 BusinessInformation에서 사업자등록증, 신분증 등등 데이터를 저장하도록 재설계 했습니다
캡슐화 문제
인증 처리, 인증 상태를 위해 BusinessInformation이 객체 참조를 하고 있는 User의 상태를 알아야함 + 데이터를 가지고 있는 인증 상태도 확인해야 하는 로직
BusinessInformation이 VerificationStatus에 대한 값의 종류와 데이터를 알아야함
인증 처리, 인증 여부를 확인하기 위해 인증 정책에게 메시지를 전송 하도록 리팩토링
인증 정책의 책임은 유저의 권한과 인증 상태 관리
해결해야 할 트레이드 오프